Fixes #25666: Add retry-with-backoff to getServiceStatus for transient failure recovery#27112
Conversation
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
There was a problem hiding this comment.
Pull request overview
This PR improves ingestion agent health-check resilience by adding Resilience4j retry-with-backoff to PipelineServiceClient.getServiceStatus(), so transient pipeline-service failures (e.g., during Airflow restarts) don’t immediately mark the agent as unavailable.
Changes:
- Moved retry-with-backoff logic into
PipelineServiceClient.getServiceStatus()and simplifiedgetServiceStatusBackoff()to delegate. - Updated retry to operate on
PipelineServiceClientResponse(retrying when HTTP code is non-200). - Added an Airflow REST client test ensuring a transient 500 followed by 200 recovers successfully and issues the expected number of requests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| openmetadata-spec/src/main/java/org/openmetadata/service/clients/pipeline/PipelineServiceClient.java | Centralizes retry-with-backoff in getServiceStatus() so all callers benefit from transient failure tolerance. |
| openmetadata-service/src/test/java/org/openmetadata/service/clients/pipeline/airflow/AirflowRESTClientTest.java | Adds regression test verifying recovery from a transient health-check failure. |
RajdeepKushwaha5
force-pushed
the
fix/25666-airflow-status-retry-on-transient-errors
branch
from
68791c8 to
9336512
Compare
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
RajdeepKushwaha5
force-pushed
the
fix/25666-airflow-status-retry-on-transient-errors
branch
from
9336512 to
f32ab33
Compare
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
RajdeepKushwaha5
force-pushed
the
fix/25666-airflow-status-retry-on-transient-errors
branch
from
f32ab33 to
0d023dc
Compare
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
RajdeepKushwaha5
force-pushed
the
fix/25666-airflow-status-retry-on-transient-errors
branch
from
0d023dc to
7a38087
Compare
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
RajdeepKushwaha5
force-pushed
the
fix/25666-airflow-status-retry-on-transient-errors
branch
from
7a38087 to
a6412ae
Compare
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
RajdeepKushwaha5
force-pushed
the
fix/25666-airflow-status-retry-on-transient-errors
branch
from
a6412ae to
2676e07
Compare
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
| public PipelineServiceClientResponse getServiceStatus() { | ||
| if (pipelineServiceClientEnabled) { | ||
| return getServiceStatusInternal(); | ||
| if (!pipelineServiceClientEnabled) { | ||
| return buildHealthyStatus(DISABLED_STATUS).withPlatform(DISABLED_STATUS); | ||
| } | ||
| PipelineServiceClientResponse response = | ||
| retryForServiceStatus().executeSupplier(this::getServiceStatusInternal); |
There was a problem hiding this comment.
PR description says getServiceStatus() now retries when the response code is “not 200”, but the implementation only retries on null or code >= 500. Please align the PR description with the actual behavior, or adjust the retry predicate if the intended behavior is to retry all non-200 responses.
Code Review ✅ Approved 10 resolved / 10 findingsImplements robust retry-with-backoff logic for service status checks, resolving multiple NPEs, configuration flaws, and incorrect exception handling. All identified issues were addressed, ensuring stable and reliable recovery from transient failures. ✅ 10 resolved✅ Bug: GetEntityVersionsTool calls non-existent CommonUtils.parseIntParam()
✅ Bug: CompareEntityVersionsTool NPE when pojoToJson returns null
✅ Bug: CompareEntityVersionsTool hardcodes table-specific fields
✅ Edge Case: MCP tools NPE when exception message is null
✅ Quality: PR includes unrelated files (notes, scripts, issue templates)
...and 5 more resolved from earlier reviews OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
|
Describe your changes:
Fixes #25666
Problem:
PipelineServiceClient.getServiceStatus()delegates togetServiceStatusInternal()with zero retry logic. A single transient REST failure (e.g.,selector manager closedduring an Airflow restart or network blip) immediately returns an unhealthy response (code 500), causing OpenMetadata to mark the Airflow agent asUNAVAILABLE. The agent never auto-recovers — a manual UI refresh or service restart is required.Root Cause: While a
getServiceStatusBackoff()method with Resilience4j retry already existed inPipelineServiceClient, it was never called by any production code. Both callers (IngestionPipelineResource.getRESTStatus()andSystemRepository.getPipelineServiceClientValidation()) callgetServiceStatus()directly, which had no retry.Fix: Move the Resilience4j retry-with-backoff logic into
getServiceStatus()itself, so every health check benefits from transient failure tolerance:getServiceStatus()now retries up to 3 attempts with 5-second backoff intervals when the response code is not 200getServiceStatusBackoff()is simplified to delegate togetServiceStatus()(avoids nested/double retry)SupplierimportTest: Added
getServiceStatusRecoversFromTransientFailuretest that enqueues a transient 500 followed by a healthy 200 and verifies the final status is healthy with exactly 3 requests (detection + failed attempt + successful retry).Type of change:
Checklist:
Fixes <issue-number>: <short explanation>Summary by Gitar
getStringParamandgetIntParamhelpers inAirflowRESTClientto safely parse configuration parameters.usernameandpasswordin the constructor, throwingPipelineServiceClientExceptionif missing.additionalPropertiesmaps gracefully.This will update automatically on new commits.